SHP Vehicle FireUp Modification #1894

Fly-Star-him · 2025-10-06T09:31:20Z

For compatibility with Delayed fire weapons.

Summary by CodeRabbit

New Features
- Frame-accurate firing sequences for infantry and units.
- Support for pausing and resuming delayed-fire sequences.
- Improved handling when units move mid-fire to safely cancel sequences.
Bug Fixes
- Fixed animation pause checks that could trigger unintended actions.
- Resolved timing mismatches that caused early/late shots in sequences.
- Corrected burst-fire behavior within firing sequences.
- Addressed rearm threshold edge cases during delayed fire.
Refactor
- Unified firing progression from timer-based to frame-based logic for more consistent timing and control.

coderabbitai · 2025-10-06T09:32:25Z

Walkthrough

Refactors firing logic from timer-based to frame-based across infantry and unit hooks. Updates TechnoExt delayed-fire API to accept explicit weapon and frame parameters, removes FiringAnimationTimer, adds a pause-aware hook, and adjusts comparisons and rearm thresholds accordingly. Minor formatting change in WeaponType code.

Changes

Cohort / File(s)	Summary of Changes
Infantry frame-based firing `src/Ext/Infantry/Hooks.Firing.cpp`	Introduces explicit frame and firingFrame computation from Animation.Value; renames local variables; updates comparisons; passes WeaponType and frame values to the new multi-arg HandleDelayedFireWithPauseSequence; adjusts rearm threshold to use fireUp.
Techno delayed-fire API and state `src/Ext/Techno/Body.h`, `src/Ext/Techno/Body.cpp`, `src/Ext/Techno/Hooks.cpp`	Removes FiringAnimationTimer member and constructor init; updates HandleDelayedFireWithPauseSequence signature to include WeaponTypeClass* pWeapon, frame, and firingFrame; replaces Animation.Value comparisons with frame-based ones; removes FiringAnimationTimer from serialization path; adds UnitClass_AI_AnimationPaused hook using DelayedFireSequencePaused; replaces FiringAnimationTimer.Stop() with ResetDelayedFireTimer().
Unit firing refactor `src/Ext/Unit/Hooks.Firing.cpp`	Replaces timer-driven firing with CurrentFiringFrame tracking; initializes and updates frames via arithmetic; early retrieval of WeaponType and extension; computes frame/firingFrame for fire-up; delegates to updated TechnoExt::HandleDelayedFireWithPauseSequence; adds branch for pause-delayed firing when fire-up inactive; uses -1 sentinel to terminate sequence on movement.
WeaponType formatting `src/Ext/WeaponType/Body.cpp`	Non-functional line break adjustment; no behavior changes.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Unit as UnitClass
  participant Techno as TechnoExt
  participant Weap as WeaponTypeClass
  participant Anim as Animation

  Note over Unit,Techno: Frame-based firing progression (replaces timer-driven)
  Unit->>Anim: Read Animation.Value
  Unit->>Unit: Compute frames, CurrentFiringFrame
  Unit->>Weap: Resolve weapon & extension
  Unit->>Unit: Compute frame & firingFrame

  alt Fire-up active
    Unit->>Techno: HandleDelayedFireWithPauseSequence(pThis, pWeapon, idx, frame, firingFrame)
    alt Sequence paused
      Techno-->>Unit: SkipFiring (paused)
    else Frame matches firingFrame
      Techno-->>Unit: Proceed to fire
    end
  else Fire-up inactive
    Unit->>Techno: HandleDelayedFireWithPauseSequence(... for pause-only)
    Techno-->>Unit: Skip or continue
  end

sequenceDiagram
  autonumber
  participant Engine as Game Engine
  participant Techno as TechnoExt
  participant Hook as UnitClass_AI_AnimationPaused

  Engine->>Hook: Animation tick
  Hook->>Techno: Check DelayedFireSequencePaused
  alt Paused
    Hook-->>Engine: SkipGameCode (0x7363DE)
  else Not paused
    Hook-->>Engine: Continue normal flow
  end

  Note over Engine,Techno: On target change
  Engine->>Techno: ResetDelayedFireTimer()

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

I hop through frames, not ticking time,
A carrot clock for every rhyme.
Pause the burst, then spring to fire—
My whiskers twitch with coded choir.
Frames align, the targets know:
I thump, I launch—then off I go! 🥕🚀

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The title highlights FireUp modifications, which is indeed a central aspect of the changeset, but it specifies only SHP vehicles and omits the extensive frame-based firing logic updates applied to infantry and general units. As a result, it partially reflects the pull request’s content but does not capture its full scope.
Docstring Coverage	✅ Passed	No functions found in the changes. Docstring coverage check skipped.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 3

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 106d2b8 and 67e0eed.

📒 Files selected for processing (6)

src/Ext/Infantry/Hooks.Firing.cpp (2 hunks)
src/Ext/Techno/Body.cpp (2 hunks)
src/Ext/Techno/Body.h (1 hunks)
src/Ext/Techno/Hooks.cpp (2 hunks)
src/Ext/Unit/Hooks.Firing.cpp (3 hunks)
src/Ext/WeaponType/Body.cpp (1 hunks)

🔇 Additional comments (8)

src/Ext/Techno/Body.h (1)

271-271: Verify all call sites have been updated to match the new signature.

The method signature change from 3 parameters to 5 parameters is a breaking API change. While the AI summary indicates updates in Body.cpp, Hooks.cpp, Infantry/Hooks.Firing.cpp, and Unit/Hooks.Firing.cpp, please confirm that all callers of HandleDelayedFireWithPauseSequence have been updated to provide the new pWeapon and frame parameters.

Run the following script to verify all call sites:

src/Ext/Techno/Hooks.cpp (1)

996-997: Verify ResetDelayedFireTimer() exists in TechnoExt.

The refactor replaces FiringAnimationTimer.Stop() with ResetDelayedFireTimer(), which aligns with the timer-to-frame migration described in the PR summary. Ensure this method is implemented in TechnoExt.

Run the following script to verify the method exists:

src/Ext/Infantry/Hooks.Firing.cpp (5)

91-91: LGTM! Variable rename improves clarity.

Renaming firingFrame to fireUp more accurately reflects that this is the base fire-up frame from the register, not the calculated firing frame. This improves readability, especially since firingFrame is later recalculated as fireUp + cumulativeDelay.

118-119: LGTM! Explicit frame calculations improve clarity.

Extracting the current animation frame into frame and calculating firingFrame upfront makes the logic much clearer and easier to maintain. This is a good refactoring that replaces implicit calculations with explicit, reusable variables.

124-124: LGTM! Simplified condition is much clearer.

Replacing Animation.Value == firingFrame + cumulativeDelay with frame == firingFrame significantly improves readability. The pre-calculated variables make the intent immediately clear without requiring readers to mentally evaluate the expression.

131-131: LGTM! Correct variable usage after rename.

The rearm threshold check correctly uses fireUp (the base frame) instead of the calculated firingFrame. This maintains the original logic: checking whether the next shot's base frame would exceed the sequence frame count, which is the right behavior for determining when to start the rearm timer.

121-121: Verify that FiringAITemp::WeaponType is initialized before use.

The updated API call passes FiringAITemp::WeaponType as a parameter, but its initialization is not visible in the provided code. According to the AI summary, weapon type is looked up early in the FireUp path. Please ensure this value is properly initialized before this call to avoid undefined behavior.

src/Ext/Techno/Body.cpp (1)

699-699: Verify frame parameter correctness across callers.

The change from pThis->Animation.Value to the frame parameter aligns with the timer-based to frame-based refactor described in the AI summary.

Run the following script to verify all callers pass appropriate frame values:

src/Ext/Techno/Body.cpp

src/Ext/Techno/Hooks.cpp

src/Ext/Unit/Hooks.Firing.cpp

github-actions · 2025-10-06T09:38:13Z

Nightly build for this pull request:

compiled-dll-9d6513ae13bbad2634aa95809fabf61be1e03b1e.zip
These artifacts will expire in 90 days and will not be available for download after that time.

This comment is automatic and is meant to allow guests to get latest nightly builds for this pull request without registering. It is updated on every successful build.

update code

67e0eed

Update Hooks.Firing.cpp

9bf42a0

coderabbitai bot reviewed Oct 6, 2025

View reviewed changes

src/Ext/Techno/Body.cpp Show resolved Hide resolved

src/Ext/Techno/Hooks.cpp Show resolved Hide resolved

src/Ext/Unit/Hooks.Firing.cpp Show resolved Hide resolved

Fly-Star-him and others added 4 commits October 11, 2025 17:05

Merge branch 'Phobos-developers:develop' into fireupfix

8c785ab

Merge branch 'Phobos-developers:develop' into fireupfix

16909bf

Merge branch 'develop' into fireupfix

9f01e5c

codestyle

fe62d71

Coronia added Tested Minor Minor feature and/or fix, not a lot of changes or they are not significant ⚙️T1 T1 maintainer review is sufficient No Documentation Needed No documentation needed whatsoever labels Oct 13, 2025

Coronia approved these changes Oct 13, 2025

View reviewed changes

Coronia merged commit 2b098a4 into Phobos-developers:develop Oct 13, 2025
15 of 18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

SHP Vehicle FireUp Modification #1894

SHP Vehicle FireUp Modification #1894

Uh oh!

Fly-Star-him commented Oct 6, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Oct 6, 2025 •

edited

Loading

Review ran into problems

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Oct 6, 2025 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

SHP Vehicle FireUp Modification #1894

SHP Vehicle FireUp Modification #1894

Uh oh!

Conversation

Fly-Star-him commented Oct 6, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Poem

Pre-merge checks and finishing touches

Review ran into problems

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Fly-Star-him commented Oct 6, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Oct 6, 2025 •

edited

Loading

github-actions bot commented Oct 6, 2025 •

edited

Loading